feat: send navbar clicks made before analytics loads through the beacon - #909

Review: approve ✅

This is a small change that follows existing patterns: navbar clicks made before analytics is ready now go through the same unload-safe beacon that useDownloadClick uses.

What I checked

• No double counting. Each click takes exactly one path. When analytics is ready it goes through track and returns; otherwise it goes through the beacon. A beacon click never enters the analytics-next buffer, so it cannot be sent again once Segment finishes loading.
• The beacon follows the same rules that keep the SDK off.
  • Bots: @dcl/hooks@2.1.0 does export isBotClient, and its AnalyticsProvider uses the same check to skip loading.
  • Exempt sessions: postSegmentEvent deliberately skips the exempt-path check (bypassExemptPathGate), so the explicit isAnalyticsDisabledForSession() call is required, and it is there.
  • No writeKey: postSegmentEvent already returns early.
  • Both checks run before ensureSegmentAnonymousId(), so bots and exempt sessions never get an ajs_anonymous_id cookie written.
• The navbar links are plain href anchors (full page loads), so the beacon is the right transport for them.
• Tests. The afterEach with jest.resetAllMocks() plus the new beforeEach keep each case isolated. The new cases cover a person, an exempt session, a bot, and analytics being ready. The ref pattern and the [track] dependencies match useDownloadClick and useDeferredTrack. No unused imports.

Minor findings (P2, not blocking)

1. P2: sign_in, jump_in and middle-clicks into a new tab do not leave the page, but they also take the beacon path before analytics is ready. This includes the window where the SDK instance exists and would queue them with full context. Details inline.
2. P2: The download CTA beacon marks its events with track_deferred: true (plus track_called_at and track_delivered_at), but the navbar beacon does not. Details inline.
3. P2: No test covers a middle-click before analytics loads, or analytics becoming ready after the first render (a rerender before the click). The second is the case the ref exists for.

CI: lint, audit, title-matches-convention, lint-package-json and Vercel pass; test was still pending when I reviewed.
Conventions: the title feat: … and the branch feat/navbar-early-click-beacon follow ADR-6.
Consumer impact: none. This is an internal component change with no public surface.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Review

Verdict: Approve. I found no P0 or P1 issues.

The change does what the description says. Before analytics is ready, a click that leaves the page now goes through postSegmentEvent. A click that keeps the page open (target="_blank", ctrl/cmd/shift-click, middle-click, Jump In) is queued in useDeferredTrack and sent once Segment is ready. After that, clicks go through track.

What I checked

• isBotClient(userAgent) is exported by @dcl/hooks 2.1.0, and it is the same check AnalyticsProvider uses to keep the SDK off for bots. Together with isAnalyticsDisabledForSession(), the beacon is skipped in the same cases where the SDK never loads. This is stricter than useDownloadClick, which skips the exempt-path check on purpose for conversion CTAs. That fits navbar clicks, which are not conversions.
• Navbar links are plain <a href> elements, so a same-tab click really does load a new page. The beacon is the right transport for these clicks, and each click is sent once only: the beacon branch returns before reaching the queue.
• ensureSegmentAnonymousId() creates and saves the ajs_anonymous_id before Segment boots, so beacon clicks and later SDK events share the same anonymous id.
• trackNavbar now reads readiness from isInitializedRef, which matches useDownloadClick. Removing isInitialized from the deps is correct.
• The tests cover beacon, new tab then drain, exempt session, bot, and the ready path, plus the helper's modifier and _blank cases.

Findings

• [P2] The beacon payload has track_deferred: true but not track_called_at / track_delivered_at. useDownloadClick's beacon branch and the useDeferredTrack queue both send all three (see the inline comment).

CI: lint, audit, and title checks pass. test was still running when I reviewed.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Review: send pre-ready navbar clicks through the beacon

Verdict: Approve. I found no P0 or P1 issues. The approach is sound and every handler sends each click down exactly one path: track, the useDeferredTrack queue, or the beacon. Nothing is counted twice.

Verified

• Click classification matches what each handler actually does.
  • Navbar links are plain <a> elements, so a same-tab click does a full page load and the beacon is right.
  • Sign In ends in redirectToAuth, which calls window.location.replace. The page unloads, so the beacon is right.
  • Jump In calls preventDefault and opens the modal or launcher. The page stays, so the queue is right.
  • DesktopTabWithDropdown uses window.open(url, '_self'), which always navigates the same tab, so the default leavesPage = true is right.
  • Middle click goes through onAuxClick with false, so the queue is right.
• The beacon follows the same rules that keep the SDK off. In @dcl/hooks 2.1.0, AnalyticsProvider stays off only for an empty write key or a bot.
  • An empty write key makes postSegmentEvent return early.
  • Bots are checked with the same isBotClient, which 2.1.0 exports for this purpose.
  • The isAnalyticsDisabledForSession() check puts back the exempt-path rule that postSegmentEvent skips on purpose.
  • Neither the SDK nor the beacon has a consent or Do Not Track check, so the beacon skips nothing the SDK applies.
• Reading isInitialized through a ref avoids beaconing a click with a stale false after the render where Segment became ready. This is the same pattern as useDownloadClick.
• Security: href and action come only from static navbar config, and the body is built with JSON.stringify and sent as text/plain with credentials: 'omit'. No new PII or injection surface.

Findings (all non-blocking)

• [P2] isSameTabNavigation ignores altKey (inline).
• [P2] The deferred-beacon payload is copied from useDownloadClick and has already drifted from it (inline).
• [P2] The positional leavesPage boolean is hard to read at the call sites. trackNavbar('jump_in', undefined, false) reads better as { leavesPage: false }.
• [P2] Test gaps. The specs cover Learn (beacon), a new-tab link (queue), exempt sessions and bots. They don't cover:
  • the Sign In beacon
  • the DesktopTabWithDropdown beacon
  • the Jump In queue
  • the payload in the "analytics is ready" case (for example, that track_deferred is not set)
• [P2] Follow-up, out of scope here. The navbar is the only postSegmentEvent caller that filters bots. The download beacons (useDownloadClick, downloadTracking, downloadPageExit, downloadFunnelExit, and others) still send for bots. Moving the bot check into postSegmentEvent would apply one rule to every caller.

CI

lint, audit, lint-package-json, title convention and Vercel pass. test was still pending when this review was posted. I couldn't run the specs locally in this environment, so please confirm test is green before merging.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Summary

This is a well-scoped change. Every navbar call site is put on the right path:

• Clicks that replace the page go through the beacon. These are same-tab <a> links (logo, Learn, credits, user menu, internal dropdown items), the section tab buttons (window.open(url, '_self')) and Sign In (redirectToAuth → location.replace).
• Clicks that keep the page open wait in the useDeferredTrack queue. These are target="_blank" items, modifier clicks, middle clicks and Jump In (useHangOutAction calls preventDefault).

Each branch returns early, so a click never goes through the beacon and also gets queued, and nothing is sent twice. The beacon gate (exempt-session check plus isBotClient) matches the rules that keep the SDK off. isBotClient is exported by the installed @dcl/hooks@2.1.0. Extracting postDeferredClick into its own module does not change what useDownloadClick sends.

Security: I found no issues. The beacon path checks the exempt-page gate and the bot check before sending. The href values come from fixed navbar config and carry no user input or wallet. userId is only present after the SDK has identified the user, the same as SDK-sent events.

Verification (local, head a379f0d): jest on LandingNavbar, deferredClickBeacon and useDownloadClick: 111/111 passing. tsc --noEmit: clean. eslint on the touched files: clean. CI checks were still pending when I posted this review.

Findings

All findings are P2. None blocks the merge.

• [P2] LandingNavbar.helpers.ts:56: the parameter type is written inline in a .helpers.ts file, which CLAUDE.md forbids ("Types / interfaces: <thing>.types.ts. Never inline in .client.ts, .helpers.ts…").
• [P2] LandingNavbar.tsx:261: the ref comment claims more than the ref does.
• [P2] deferredClickBeacon.ts:8: the doc comment says callers apply the exempt and bot gates, but useDownloadClick applies neither.
• [P2] Test gaps in LandingNavbar.spec.tsx. These cases are not covered at the component level:
  • A middle click (onAuxClick → queue) before analytics is ready.
  • A ctrl/cmd click on a same-tab link through the component. Only the helper is tested for this.
  • A beaconed click followed by analytics finishing, asserting that track is not called afterwards. This would lock in the no-double-send behavior.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Review

Verdict: Approve. I found no P0 or P1 issues.

The routing is correct. Before analytics is ready, each click goes to exactly one transport (beacon or queue), never both. Once analytics is ready, every click goes through track as before. I checked each caller's leavesPage value:

• Leave the page: plain same-tab links, the desktop section tab (window.open(url, '_self'), which stays in the same tab even with a modifier key held), and both Sign In buttons (useSignInRedirect always calls redirectToAuth).
• Keep the page: _blank links, ctrl/cmd/shift/alt clicks, middle click, and Jump In (it only opens the download modal or the desktop app).

Moving the isInitialized read into a ref is safe: track keeps the same identity while analytics loads, so trackNavbar doesn't capture a stale value. The beacon gate (isAnalyticsDisabledForSession + isBotClient) uses the same conditions that keep the SDK off. It is needed because postSegmentEvent bypasses the exempt-path gate. The postDeferredClick extraction in useDownloadClick sends the same payload as before, and the download CTAs are still ungated on purpose.

Tests cover the beacon path, the queue for new-tab, modifier and middle clicks (including the flush with no double send), the exempt and bot sessions, Sign In, and all four modifier keys in isSameTabNavigation.

Minor (P2)

• P2 LandingNavbar.tsx:277: in exempt or bot sessions, clicks that keep the page are still pushed into useDeferredTrack's queue. Analytics never loads there, so the queue is never flushed. Nothing is sent, but the closures stay in memory for the life of the component. See the inline comment.
• P2 (by design, noting it for the data team): a same-tab click made while analytics.js is loaded but not ready goes out through the beacon, so it carries the beacon's context instead of the SDK's (no campaign/UTM enrichment). Analysis that splits on track_deferred = true should keep this in mind.

CI: lint, lint-package-json, title and Vercel are passing. test and audit were still running when I posted this.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Review: approve ✅

Verdict: approve. I found no P0 or P1 issues at 898f3c7.

What changed since the last round

898f3c7 moves the isAnalyticsDisabledForSession() || isBotClient(navigator.userAgent) check ahead of both deferred paths. In exempt sessions and for bots, clicks are now neither sent by beacon nor queued, so the queue no longer collects closures that never flush. The fix is correct. Both new tests middle-click, force readiness and check that nothing is sent. Middle-click is the right input to test, because before this commit it was the case that reached the queue.

What I checked

• Each caller is routed to the right path. I traced every trackNavbar / trackNavbarLink caller:
  • Beacon (the click replaces the page): same-tab links (logo, Learn, dropdown items, credits chip, desktop and mobile user menu, mobile sub-items), section tabs (window.open(url, '_self')) and both Sign In buttons (redirectToAuth).
  • Queue (the page stays open): target="_blank" items, ctrl/cmd/shift/alt clicks, middle click (onAuxClick), and Jump In (it opens a modal or the launcher).
  • Keyboard: pressing Enter fires click with button === 0 and carries the modifier flags, so a keyboard click is classified the same way as a mouse click.
• No double counting. Each branch returns early. A beacon click never enters the queue or the SDK buffer.
• The queue survives route changes. LandingNavbar is rendered by the parent <Layout> route, so it stays mounted across nested routes and the component-scoped queue is kept.
• Security (checked against the bug-bounty rules): no issues found. The beacon payload carries only static navbar config (place, event, action, section, href) plus the standard context. userId matches what the SDK sends. Exempt sessions and bots are skipped before ensureSegmentAnonymousId(), so they never get an anonymous-id cookie.
• No public API change. postDeferredClick is internal, and useDownloadClick sends the same payload as before.
• Local checks: jest on LandingNavbar, deferredClickBeacon and useDownloadClick passed 120/120. tsc --noEmit and eslint on the touched files are clean. CI (test, lint, audit) was still running when I posted this.

Minor (P2, not blocking)

• P2: LandingNavbar.spec.tsx:802. The Jump In drain test does not check how many times the click was sent (see the inline suggestion).
• P2: If analytics never becomes ready for another reason (an ad blocker blocks the settings fetch, or the write key is empty), queued new-tab clicks are never sent. Same-tab clicks may still arrive by beacon. This is no worse than before the PR, when every such click was dropped. It does skew the split between new-tab and same-tab clicks in those sessions, which is worth knowing when reading the data.
• P2: Not signed in, a visitor clicks Jump In before analytics is ready and then follows the download modal's link in the same tab, still before analytics is ready. The queued jump_in click is then lost when the page unloads. The window is narrow because analytics loads once the page is idle, so this is acceptable as is.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Review

This looks good. Each click goes through exactly one transport (beacon, queue, or track), so nothing is sent twice. The bot and exempt-session checks run before either deferred path, which fixes the case where a queued click would wait forever.

I checked how each call site is classified:

• Desktop section tab (window.open(url, '_self')), logo, Learn, Credits, mobile links, user menu: these use the beacon on a plain click and the queue for _blank or modifier clicks. Middle clicks always use the queue. ✅
• Sign In (redirectToAuth replaces the location): beacon ✅
• Jump In (prevents the default action, opens the modal or deep link; the page stays): queue ✅
• useDownloadClick → postDeferredClick is a straight extraction that sends the same payload ✅
• Anonymous id: the beacon uses ensureSegmentAnonymousId, which stores the id where analytics.js picks it up, so beacon events and later SDK events share one id. Bots and exempt sessions return before an id is created. ✅

The tests are thorough: beacon vs queue routing, exact payloads, ordering against window.open, both gates on both paths, and no double send once analytics is ready.

Findings (none block the merge):

• P2 LandingNavbar.tsx:281: the queue only lives as long as the component, so a queued click is lost if the page unloads before analytics is ready. Example: ctrl-click a link, then plain-click another. See the inline comment.
• P2 LandingNavbar.helpers.ts:58: only _blank counts as a new tab.
• P2 LandingNavbar.spec.tsx:779: window.scrollY is overridden and never restored.
• P2 nit: several beforeEach(async …) blocks in the spec are async but don't await anything (one awaits only some of its steps). This is harmless.

CI: audit, lint, lint-package-json, title and Vercel pass. test was still pending at review time.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub

Summary

Approve. This PR routes navbar clicks made before analytics is ready by whether the page survives the click. A click that replaces the page (same-tab link, Sign In, section tabs) goes out by beacon. A click that keeps the page (_blank or a named target, ctrl/cmd/shift/alt click, middle click, Jump In) waits in the useDeferredTrack queue. Exempt sessions and bots send nothing. Once analytics is ready, every click goes through track as before.

What I checked

• Every tracked navbar element (logo, Learn, Credits chip, dropdown items, user menu items, section tabs, Sign In, Jump In) takes the right path:
  • Same-tab <a> links and section tabs (window.open(url, '_self')) take the beacon.
  • Sign In takes the beacon, since redirectToAuth calls location.replace.
  • Jump In is queued, since it calls preventDefault and the page stays.
  • The notification bell, notification items, the avatar toggle, the hamburger menu and Sign Out are not tracked, so none of them can beacon by mistake.
• No double send: a beaconed click is never also queued, the queue is cleared before it is replayed, and the tests assert one track call and no beacon.
• Same rules as the SDK: the exempt-session and bot checks match the two conditions under which AnalyticsProvider stays off. postSegmentEvent still returns early when the write key is empty.
• No consent bypass: the repo has no consent, cookie-banner or Do-Not-Track code that the beacon could skip.
• href exposes nothing private: it always comes from fixed values in navbarConfig.ts or constants in LandingNavbar.tsx.
• useDownloadClick refactor: it keeps the same behaviour. The payload, the timestamps and the anonymous-id handling are unchanged, now in postDeferredClick.
• Locally: tsc -b, eslint on the changed files and jest (7 suites, 131 tests) all pass. CI was still pending when I reviewed.

Findings (P2 only, none blocking)

• [P2] src/modules/deferredClickBeacon.ts:24: ensureSegmentAnonymousId() runs before postSegmentEvent checks the write key (see the inline comment).
• [P2] src/modules/segmentBeacon.ts:147-151 (not in this diff): the NOTE there says every caller of this transport fires an explicit conversion event (download CTA click, funnel exit, download_*). That is no longer true now that navbar clicks use it too. Consider updating the comment so the reason it skips the exempt-path gate still holds, and noting that the navbar applies the gate before it calls.

I did not repeat findings from earlier rounds that were resolved or deliberately left as they are, such as the per-component queue being lost when the page unloads.

Reviewed by Jarvis 🤖 · Requested by braianj via GitHub